-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CoverArtCache refactoring + Fix scrolling lag after updating Mixxx #12009
Conversation
b8445d4
to
40a0c16
Compare
src/track/track.cpp
Outdated
static_assert(sizeof(TrackId) <= sizeof(void*)); // no locking required | ||
return m_record.getId(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why? are you sure the same assumption is true for ARM?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. There is theoretical the issue for a possible odd aliment we may assert as well, but you need to try hard to trick the compiler to achieve that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, a alignas(int) int m_value;
is the best solution without loosing pipelining because of the memory barriers in an a std::atomc_int
. I have also remove the generic value_type which gives us the guarantee that we have always int and allows fully in-lining of the DbId class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not comfortable with bugprone optimizations like this without evidence that they are needed. I also doubt that a properly used std::atomic
would be any slower than this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was originally started to implement a std::atomic based solution here. With a 'memory_order_relaxed' you can also achieve that the processor pipeline is not cleared. But than I have noticed that the load a and store calls are (probably intentional) not inline, they are real calls. std::atomic are used for atomic operations like read and increments and memory briers, all this not needed here. We would need a lot of boilerplate code to achieve something with std::atomic that is reliable out of the box.
Here we really just want to read a single integer as a whole that is in addition almost constant. This is already a thing that cannot be interrupted = atomic. The compiler only uses unaligned integers when it is forced to. See also https://stackoverflow.com/a/70967180 and https://learn.microsoft.com/en-us/cpp/cpp/alignment-cpp-declarations?view=msvc-170
I have found a one header implementation of an atomic int that also uses aligneas(int): https://github.com/wingfiring/xirang/blob/eda84647cca476a29e8d785b8fd8ceda78bb952f/trunk/xirang/backward/atomic.h
so we are in a good companion.
But anyway now with alingeas(int) we have sorted out this case as well. So why do you still think this is bugprone. What is the scenario that may fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was originally started to implement a std::atomic based solution here. With a 'memory_order_relaxed' you can also achieve that the processor pipeline is not cleared. But than I have noticed that the load a and store calls are (probably intentional) not inline, they are real calls.
Are you sure, they're fully inlined on -O1
on all compilers. https://compiler-explorer.com/z/n3MbKqr3q
There is no reason why they couldn't be inlined unless atomics where using dynamic dispatch (which they absolutely aren't, considering how performance-sensitive they are).
We would need a lot of boilerplate code to achieve something with std::atomic that is reliable out of the box.
Why would that be? Just make the member atomic and then .load()
and .store()
to it.
This is already a thing that cannot be interrupted = atomic.
I'm worried the assumption might break, silently resulting in a race-condition. I'd rather have the fallback to slower behavior, than undefined behavior.
I have found a one header implementation of an atomic int that also uses aligneas(int): https://github.com/wingfiring/xirang/blob/eda84647cca476a29e8d785b8fd8ceda78bb952f/trunk/xirang/backward/atomic.h
so we are in a good companion.
That project has been dead for 6 years, I doubt they did any work to validate whether that actually works on every platform they support, which are probably even fewer platform than we support.
But anyway now with alingeas(int) we have sorted out this case as well. So why do you still think this is bugprone. What is the scenario that may fail?
Because on paper alignas
just guarantees alignment not atomicity. The conclusion you've drawn that aligned access means atomic access is implicit but not strong enough to not silently break. We either need to be able to assert this (which we can't afaik), or have it documented rigidly for all platforms we support (and might support, I don't want mixxx be unusable on RISC-V because we relied on a bunch of ARM and x86 specific behavior).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to convince me that alignas
guarantees atomicity, just link me the section in the standard (or even cppreference would be fine) that supports your assumption. But I don't think that assumption is true in all cases and thus its also not defined in the standard. As such, we can't use it blindly.
If its only defined for some CPU architectures, fine, that works too, but then we need to assert that we only run on those CPU architectures, etc. Do you see the pattern? just linking to some code and stackoverflow articles describing what might happen on some systems doesn't let us draw conclusions over all systems!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I have committed the std::atomic_int version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I will try to take a look, soon-ish.
Is s class always integer alligned? Probably not. I think we need to assert that. |
This doesn't compile here: |
Now I can build locally. Everything works as before, except the Coverart-Tooltip in the library, which does no longer appear. I guess this is intended for this intermediate step. |
It should appear after the second try. The first try fills the cache. This should however the same in 2.4. |
No, this was the behavior before. With this PR a tooltip for the CoverArt does not appear at all. |
Oh, there was missing ! introduced in the last minute. Now everything should be like before, just faster. |
I need to leave a comment here.
Now it has become an opinionated |
@@ -22,35 +22,36 @@ | |||
// not add any additional state (= member variables). Inheritance is | |||
// only needed for type-safety. | |||
class DbId { | |||
protected: | |||
public: | |||
// Alias for the corresponding native type. It keeps the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment explained this intended purpose. But it has been and deleted.
I tested this, everything works now as before, but the CoverArt loading while scrolling through the library feels to be faster now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blocking this PR for now as I don't agree with the changes. I'll elaborate asap.
I wanted to use the TrackId to check if it is a temporary track or not. However at that point the temporary track used for lookup the metadata is out of scope. So it means the original demand of optimize the locking is not there. This means we can revert the introduction of an atomic here. |
.. in favour of the already existing toString() and toVariant() interface. This hides the internal int value entirely.
@Swiftb0y Ok, the int is gone. Please have a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't comment on coverartcache
, coverartdelegate
and basetracktablemodel
, but my concerns over DbIb have been mostly resolved.
can't comment on coverartcache
, coverartdelegate
and basetracktablemodel
as I'm completely unfamiliar with the code in question. Someone else please take over.
The clazy failure is already fixed in 2.4. |
@daschuer Please fix the build failures - by rebase, or whatever is needed |
All green again. |
@JoergAtGithub if you're willing to sign off on the coverartcache, coverartdelegate and basetracktablemodel changes, feel free to merge this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed the code and found only some minor nitpicks. I also build it on Windows 11 and it works as expected.
Co-authored-by: JoergAtGithub <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Waiting for CI
All green again |
What did you push here? |
I have amended the last commit, because there was a code style issue. |
Oh no, I see. I have messed with the follow up PTRs. |
Restored the original state from reflog. |
Thank you! |
This is a prerequisite to fix #11131
It consolidates a grown interface with a hard to use
enum class Loading
that was passed in most cases redundant to the different inputs to a function. To use it right you needed to pass the correct loading value with the correct set of parameters.This has been now replaces with single purpose functions which should be self-explainable.
The original universal interface has been mad private.
To make this work, a refactoring of the migration from legacy hash was required.
This is now always done in the worker thread, to get around the hard to understand extra information of
requestedCacheKey
andcoverInfoUpdated
in theCoverFound()
signal. This should improve the library scrolling after updating from a legacy Mixxx version.I have tried hard to do this PR in single testable commits, to allow a good review.
I have two follow up in my stash: